Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support type-dependent search result highlighting via CSS #12474

Merged
merged 8 commits into from
Aug 11, 2024

Conversation

timhoffm
Copy link
Contributor

@timhoffm timhoffm commented Jun 24, 2024

Edit: Summary (Update: 25.07.2024)

This adds result type-dependent HTML classes to the <li> elements of the search result list. Implemented types / classes are:

  • context-index: If the search term is found in an index such as the glossary
  • context-object: If the search term a code object such as a python function
  • context-text: If the search term is found in plain documentation text
  • context-title: If the search term is found in a heading

The basic style intentionally does not apply styling to these classes to not interfere with derived themes. We encourage theme authors to apply suitable styling to the results list via CSS as they see fit. Check sphinx13.css in this PR as an example.


It's helpful to visually distinguish different content types.

This PR adds context to the javascript result entries (one of "title", "index", "object", "text" - please check that these names make sense, I've inferred them from the JS context) and adds them as a class to the <li> item in the result list. This allows styling via CSS.

For simplicity, I've styled with unicode symbols, which should give a decent look without the need to ship our own symbols. Derived themes have the ability to adapt this via CSS and use their own icons if they want to.

Before / after:

image         image

@jayaddison jayaddison added type:enhancement enhance or introduce a new feature type:proposal a feature suggestion html search labels Jun 24, 2024
background-image: url(file.png);
background-repeat: no-repeat;
background-position: 0 7px;
list-style: "\25A1"; /* Unicode: White Square */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether it might make sense to be slightly more explicit and define these using the CSS list-style-type property?

Also, to check: is the white-square default here intended to highlight cases where sites have upgraded their Sphinx CSS but not rebuilt their searchindex.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a CSS expert. I was anticipating that list-style is easier to override for downstream themes, e.g. list-style: url(image.svg), but just a guess. If you say list-style-type is better, I'm happy to change.

I've added the empty square as a catch-all fallback; e.g. could be that somebody rewrites search and includes a new type, but forgets the corresponding CSS. Likely other things can happen and the empty square as as placeholder is better than nothing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a CSS expert. I was anticipating that list-style is easier to override for downstream themes, e.g. list-style: url(image.svg), but just a guess. If you say list-style-type is better, I'm happy to change.

Me neither, and your intuition may be correct. I based my suggestion on a sense that list-style-type is slightly less ambiguous (list-style does accept the same values, but can also be set to an image URL), and also a small note on the Mozilla MDN page:

The list-style property is specified as one, two, or three values in any order. If list-style-type and list-style-image are both set, the list-style-type is used as a fallback if the image is unavailable.

I've added the empty square as a catch-all fallback; e.g. could be that somebody rewrites search and includes a new type, but forgets the corresponding CSS. Likely other things can happen and the empty square as as placeholder is better than nothing.

That makes sense. A concern I have with it is that the same (or a similar) symbol is used on some systems when a unicode codepage/symbol is not found in the relevant font, and that could make problems slightly trickier to track down. Perhaps we could set the value to initial (initial browser default)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A concern I have with it is that the same (or a similar) symbol is used on some systems when a unicode codepage/symbol is not found in the relevant font

Our assumptions in the HTML search is that the user has a modern browser, and hence we should be confident in using Unicode -- every modern browser is able to display emoji, etc.

Comment on lines 122 to 124
ul.search li.title {
list-style: "\1F4C1"; /* Unicode: File Folder */
}
Copy link
Contributor

@jayaddison jayaddison Jun 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only annotation symbol that I find slightly confusing / out of context, personally.

It does sorta logically fit that it's for use to represent that titles are 'containers' in some sense for text content.

I'm browsing unicode character references to try to find something else that could represent text/documents, headings/titles, and/or the potential for containership/nesting.

Edit: remove repetitive phrasing

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's a controversial misuse, but perhaps the paragraph symbol (called Pilcrow, as I learned today)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(or we could try to bring the capitulum back into style)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another alternative could simply be to style title and text matches identically to begin with.

I'll try to spend a bit more time thinking about this before adding any further thoughts/comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea behind folder was "structuring element" but I'm not too convinced of it either.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On reflection: I'd recommend keeping this simple, and using the same styling for both title and text search results - that is, 1F4C4 (unicode: page facing).

We could still emit the CSS classes title and text in the result list, allowing downstream sites to customize their display to suit their projects if they choose.

Some reasoning for this suggestion: with a default Sphinx configuration and HTML build, it is possible to infer from each listed search result whether the input search query term(s) matched in the corresponding document's title and/or content body. In my personal opinion it is a nice addition to be able to distinguish between glossary items and programming objects, but seems lower priority to distinguish page title matches from other page content matches.

@jayaddison

This comment was marked as resolved.

@jayaddison
Copy link
Contributor

Maybe nitpicky: the horizontal margin/padding between the icons and the search result titles has increased; is there a way to reduce that back to near the original spacing?

@wlach
Copy link
Contributor

wlach commented Jun 25, 2024

Might be interesting to see how this works with some popular themes e.g. furo, readthedocs, and sphinx-book-theme. I suspect it'll look fine but can't hurt to double check.

@timhoffm
Copy link
Contributor Author

Maybe nitpicky: the horizontal margin/padding between the icons and the search result titles has increased; is there a way to reduce that back to near the original spacing?

Done:

image

@timhoffm
Copy link
Contributor Author

timhoffm commented Jun 25, 2024

Might be interesting to see how this works with some popular themes e.g. furo, readthedocs, and sphinx-book-theme. I suspect it'll look fine but can't hurt to double check.

Good point. They all set ul.search {list-style: none}, which this ul.search li.* {list-style: ...} will superseed; e.g. Furo:
image

That may be ok, but still we're pushing unexpected styling, and e.g. the spacing is too narrow for furo.
If we do that, we should give them a heads up. They can either set ul.search li {list-style: none} themselves. or maybe the also want to style depending on the type.

The alternative would be only to implement the class now, and change the CSS later, which gives downstream more time to react.

@timhoffm
Copy link
Contributor Author

timhoffm commented Jun 27, 2024

Alternative idea:

  • Remove any styling from the basic theme, so that it's a simple bulleted list (the current constant background image does not add much and it feels reasonable to keep basic theme styling minimal).
  • Still add the html classes in basic via search
  • Actual CSS styling is up to the downstream themes; in particular I would add the styling proposed to sphinx13 and alabaster. Other themes should decide for themselves.

What do you think?

@jayaddison
Copy link
Contributor

If we roll the theming out 'centrally' (in the basic) theme, I'd expect that there could be some display/layout issues for some downstream themes; hopefully those problems would be reported back to us and we could resolve them in a cross-theme-compatible manner (but those are assumptions!).

If we roll out only the HTML class names, my guess is that many themes will not notice or react to the possibility to style these elements -- that's fine, but I do think that there is a genuine user-facing improvement possible here. I also think that there's some risk -- but also some opportunity -- if fragmentation of result icon theming occurs.

In either scenario I'd be hopeful for some future requests for additional class names / tweaks to improve the categories further.

I don't have a strong preference either way yet; just trying to consider the likely outcomes at the moment.

One other thought: the basic.css_t file is a Jinja template, and can contain some basic logic; we could theoretically add a Sphinx HTML search config option to control the theming. I don't know whether that's worth it (another config setting to support, potentially over a long timescale), but again mentioning it so that we can consider it.

@timhoffm
Copy link
Contributor Author

From the themes listed at https://sphinx-themes.org/, I pulled the GitHub stars as a proxy measure of usage. ~10 themes cover most of the user space.

image

So the amount of impact is managable either way: If we supply icons by default, we can check them. If we only provide classes, we can notify them that they could do styling.

From the top 7 themes only Alabaster and Bootstrap keep the original search result styling. The others have chosen to explicitly style the result themselves (often just using list-style: none).

Conceptually, I find it attractive to include as little styling as possible in the basic theme. It should give the structure, appearance should be handled by the derived themes. Us putting icons into basic makes it potentially harder for them. I believe my proposal above minimizes negative impact.

@jayaddison
Copy link
Contributor

@timhoffm Thanks for providing that contextual data. I'm mostly in agreement with your suggestion about leaving the basic theme as-is - providing a gradual opt-in approach seems sensible.

An aspect I'm puzzling over is whether we should be even more cautious about affecting downstream, and not remove the file.png icon alongside each result (redundant though it does seem).

Yet one more thought: the choice of tag names does seem important, as you highlighted originally. In particular object currently strikes me as an area where we might be able to do better -- perhaps passing the Sphinx domain and object-type (e.g. py:module, c:function) could allow additional useful theming.

@jayaddison
Copy link
Contributor

An aspect I'm puzzling over is whether we should be even more cautious about affecting downstream, and not remove the file.png icon alongside each result (redundant though it does seem).

I think that was too cautious and hesistant/change-resistant of me. I'm not an accessibility expert but I would expect that the file.png icon appearances in the result HTML are noise that people may want to filter out; we should remove the icon since it doesn't add any value visually either, IMO.

@timhoffm timhoffm force-pushed the search-icons branch 3 times, most recently from b4affb1 to 0ac6880 Compare June 30, 2024 15:49
@timhoffm
Copy link
Contributor Author

timhoffm commented Jun 30, 2024

I've changed the basic theme to not have any styling. The effect can be seen as follows at the example of alabaster, which uses the default settings for search:

old / new:

grafik grafik

The sphinx13 theme still got the icon styling. I plan to provide a PR for that on alabaster as well, once this PR is merged.

Yet one more thought: the choice of tag names does seem important, as you highlighted originally. In particular object currently strikes me as an area where we might be able to do better -- perhaps passing the Sphinx domain and object-type (e.g. py:module, c:function) could allow additional useful theming.

IMHO we anyway need object to allow easy styling for the whole category. We don't want downstream themes to force adding a long exhaustive list in CSS .py:module .py:class .c:function ... { }. For simplicity, I've now started only with object. One can later expand this to have more specific py:module etc. classes alongside object.

@jayaddison
Copy link
Contributor

The sphinx13 theme still got the icon styling. I plan to provide a PR for that on alabaster as well, once this PR is merged.

+1 to this approach (applying the change to the project's own documentation theme first / milling our own grain)

Copy link
Contributor

@jayaddison jayaddison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me - thanks @timhoffm!

image

@jayaddison
Copy link
Contributor

Ah, one more thing @timhoffm - I always forget this - please could you add an entry to the CHANGES.rst file to describe this feature?

@timhoffm timhoffm changed the title ENH: Show type-dependent icons on search result entries ENH: Support type-dependent search result highlighting via CSS Jul 1, 2024
@jayaddison
Copy link
Contributor

I'm taking a pause here @timhoffm - I'll try to continue this review (and to actually test the suggestions I'm making before adding them, when doing that) later today or tomorrow.

@timhoffm
Copy link
Contributor Author

By the way: if your local workflow isn't too disrupted by it, it's generally preferable to accumulate commits (including merge commits if necessary to stay up-to-date with the master branch) rather than force-push; pull requests are generally (always?) squashed into a single commit at merge-time. It's not mentioned in the contributing guide, but perhaps should be.

I personally find PRs with "merge master into feature" more difficult to review, because you mix irrelevant changes from main into the feature code. Therefore, I typically rebase. But if you prefer merges, I can do that as well.

Would be good to mention this in the Contributing docs if you have preferences for a specific workflow.

@timhoffm
Copy link
Contributor Author

timhoffm commented Jul 25, 2024

I'm taking a pause here @timhoffm - I'll try to continue this review (and to actually test the suggestions I'm making before adding them, when doing that) later today or tomorrow.

@jayaddison Thanks for the review! Feel free to directly push any changes. I feel, I've done all that I can for this PR, and in particular if it's JavaScript, I won't be any help.

@AA-Turner
Copy link
Member

@jayaddison at some point, let me know when you're happy with this PR and/or what your outstanding concerns are. This will go into 8.1.

A

@jayaddison
Copy link
Contributor

@jayaddison Thanks for the review! Feel free to directly push any changes. I feel, I've done all that I can for this PR, and in particular if it's JavaScript, I won't be any help.

You're welcome @timhoffm - please note that I don't have committer rights here, so I can't push to the branch, but I may add a few more suggestions when ready, or open a pull request on your fork of the repo.

@jayaddison at some point, let me know when you're happy with this PR and/or what your outstanding concerns are. This will go into 8.1.

Ok, thank you @AA-Turner. The three remaining items I have in mind are:

  1. Some phrasing adjustments for the documentation.
  2. If possible, making the stylesheets theme config option in the documentation a reference/hyperlink (maybe non-trivial / could require separate supporting work).
  3. The JavaScript enum handling questions, as found in previous subthreads here.

@AA-Turner
Copy link
Member

The three remaining items I have in mind are:

Thanks. I'm minded to merge this as-is and potentially we can tackle those points in follow-ups, given none seem blockers to the feature itself? If you'd prefer that this PR stay open though, no worries.

A

@jayaddison
Copy link
Contributor

jayaddison commented Jul 31, 2024

The three remaining items I have in mind are:

Thanks. I'm minded to merge this as-is and potentially we can tackle those points in follow-ups, given none seem blockers to the feature itself? If you'd prefer that this PR stay open though, no worries.

I don't think they're worth considering as blockers either, merging is fine with me 👍

@AA-Turner AA-Turner merged commit 646a5d7 into sphinx-doc:master Aug 11, 2024
20 of 21 checks passed
@jayaddison
Copy link
Contributor

🎉 thanks @timhoffm @AA-Turner!

@timhoffm timhoffm deleted the search-icons branch August 12, 2024 05:24
@AA-Turner
Copy link
Member

Something seems to be off: https://www.sphinx-doc.org/en/master/search.html?q=directive

image

Note every result is "White Square"...

A

@timhoffm
Copy link
Contributor Author

Has there been a change to the search results meanwhile? They seem to be grouped now: All glossary entries (directive, master document, object) are together, all entries on a page are together, ...

I will check in more detail, but that will take a week.

@AA-Turner
Copy link
Member

Perhaps @wlach or @jayaddison might know?

A

@jayaddison
Copy link
Contributor

@AA-Turner @timhoffm it's worth noting that the www.sphinx-doc.org website uses ReadTheDocs' cloud-hosted search implementation -- it may have begun grouping results, but that isn't a feature that is (yet!) implemented in the standalone/native search within Sphinx.

That probably explains why the results don't have any context-... CSS class attributes on them at all, and hence why we're seeing the default white-squares. Does that seem to make sense? (I haven't confirmed this in precise detail, but it's my theory so far)

@jayaddison
Copy link
Contributor

One more note: inspecting the HTML, I noticed that we already use the term context in the CSS classes to refer to the text snippet / summary text shown alongside each search result; see the code here:

summary.classList.add("context");

My apologies for not noticing that during the discussion about what to name the type/kind/context classname. I now feel that we should disambiguate the two (and we do still have time to do that, since this change is not-yet-released). I think kind-... is probably the way to go after all (and I will have to learn to be less lazy about the way that I ask about non-optimal search results during bugreports).

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2024
@AA-Turner AA-Turner modified the milestones: 8.x, 8.1.0 Oct 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
html search type:enhancement enhance or introduce a new feature type:proposal a feature suggestion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants